osctrl: follow-up fixes for #813 — per-service --auth defaults + carve path SQL escape#817
Conversation
|
Hi @javuto — caught your "leave it like this, including the usage" on #813 this morning, but it landed after #817 was already up with the short Usage form. Apologies for the cross-thread mismatch — I didn't have your preference yet when I drafted #817. The per-service flag split in #817 (api=jwt, admin=db, tls=none) addresses your "tls is AuthNone for sure" remark and isn't affected. The only piece that diverges from your latest guidance is the Usage string on osctrl-api's Your preference (long, what #813 shipped with): Currently in #817 (short): Happy to amend #817 to restore the long form. Before I do, I want to surface the trade-off so you can pick with full context — I went with the short form for security-leaning reasons that are worth a moment. Why the short Usage in #817:
Why the long Usage is reasonable:
My read: the security delta is small — ~15 seconds of friction to disable auth. The guard catches the dangerous outcomes either way. The choice is really UX-vs-defense-in-depth at the margin, and you have better visibility into which side of that margin matters in practice. Tell me which way to go and I'll amend. |
Follow-up on jmpsec#813 (merged) addressing two review threads: == Per-service --auth defaults == The shared `initServiceFlags` was applying a single `--auth` default across all three services, which produced the wrong `--help` output for osctrl-tls (which authenticates osquery nodes via the per-env enroll secret, not via this flag — `none` is the correct default for it). Split the `--auth` flag out of `initServiceFlags` into a new `initAuthFlag(params, defaultValue)` helper and call it from each service's `Init*Flags` with the semantically correct default: - osctrl-api → `jwt` (closes U-AUTH-1, the audit-flagged "unauthenticated by default" finding) - osctrl-admin → `db` (its existing browser login form) - osctrl-tls → `none` (osquery nodes auth via enroll secret; tls's main.go does not read this field) Usage text reverted to "Authentication mechanism for the service" per review feedback — the env-var note is intentionally not in help text; operators who set `--auth=none` on osctrl-api see the actionable error message at startup from `guardAuthMode` instead. == Carve path: replace regex gate with SQL string-literal escape == The previous `ValidCarvePath` regex blocked legitimate paths that contain spaces, parentheses, or non-ASCII characters (e.g. `C:\Program Files\Common Files\app.log`, `/Library/Application Support/com.example/cfg`, `/home/álvaro/notes.txt`). Replaced with a splice-site defense: - `escapeSQLString` doubles single quotes so an attempted injection like `'; SELECT 1; --` becomes the literal contents of the SQL string literal — there is no way to break out of the surrounding quotes. - For glob mode, `escapeLikePattern` escapes the LIKE meta-chars (`\`, `%`, `_`) with `\` so pre-existing LIKE characters in the path stay literal, then `*`/`?` map to `%`/`_`. The query emits `ESCAPE '\'` so the parser respects the escapes. - `globToLike` runs the LIKE escape first, then the glob mapping, then the SQL-quote escape — the order matters and is documented. Removed `ValidCarvePath` / `validCarvePath` and the precheck call in `CarvesRunHandler`. Test coverage: - injection attempts on both exact and glob branches - legitimate Windows / macOS / UTF-8 paths that the old regex would have rejected - glob mapping with literal-meta-char escapes - SQL shape sanity for happy paths Verified locally: - go build ./... (clean) - go vet ./... (clean) - go test ./... (all packages green; 4 new carve tests pass) - osctrl-api/admin/tls --help all show the correct per-service auth default - Live smoke on a dev stack against real Postgres: 4 carve requests with Windows-with-spaces, macOS Application Support, UTF-8, and the classic injection string all land as expected — the injection's single quote is doubled to `''` in the persisted SQL, and SQLite parses the result as one literal string.
46f6777 to
39ca450
Compare
Follow-up to #813 addressing the two open review threads that landed after the merge:
1. Per-service
--authdefaults#813 flipped the shared
--authdefault tojwt, but the flag lives ininitServiceFlagswhich is called by all three services. That produced misleading--helpoutput for osctrl-tls (which authenticates osquery nodes via the per-environment enroll secret, not via this flag).Split
--authout into a newinitAuthFlag(params, defaultValue)helper and call it from eachInit*Flagswith the correct default per service:jwtdbnonemain.godoesn't read this fieldUsagetext reverted to the original"Authentication mechanism for the service"per your other comment on #813. TheOSCTRL_INSECURE_NO_AUTHenv-var note is no longer in help text — operators who set--auth=noneon osctrl-api see the actionable error message at startup fromguardAuthMode(unchanged from #813).2. Carve path: replace regex gate with SQL string-literal escape
The previous
ValidCarvePathregex (introduced in #813) rejected legitimate paths containing spaces, parentheses, or non-ASCII characters —C:\Program Files\Common Files\app.log,/Library/Application Support/com.example/cfg,/home/álvaro/notes.txtall failed it.Replaced with a splice-site defense in
GenCarveQuery:escapeSQLStringdoubles single quotes —'; SELECT 1; --becomes''; SELECT 1; --, parsed by SQLite as the literal contents of one string, no way to break outescapeLikePatternescapes LIKE meta-chars (\,%,_) so existing characters in the path stay literalglobToLikeruns LIKE-escape first, then maps*/?→%/_, then SQL-quote-escapes; order documentedESCAPE '\'so the parser honors the escape characterRemoved
ValidCarvePath/validCarvePathand the precheck call inCarvesRunHandler. Test coverage:%,_,\)Verified
go build ./...cleango vet ./...cleango test ./...green across all packagesosctrl-api/admin/tls --helpall show the correct per-service auth defaultTest plan
osctrl-tls --help | grep '\-\-auth'showsdefault: "none"C:\Program Files\app.log); confirm the carve completes and the path round-trips